Skip to content

Migrate layout to Typescript#2452

Merged
kaixin-hc merged 2 commits into
MarkBind:masterfrom
yiwen101:layout_ts
Mar 14, 2024
Merged

Migrate layout to Typescript#2452
kaixin-hc merged 2 commits into
MarkBind:masterfrom
yiwen101:layout_ts

Conversation

@yiwen101

@yiwen101 yiwen101 commented Mar 10, 2024 •
edited by kaixin-hc
Loading

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

work on #1913. Credit to #2164 for some of the works. Did not commit to #2164 to avoid the git rebase issues currently haunting it. Same modification as #2447, but cleaner git history.

Overview of changes:
Update the use of layout manager in Site and Page;
Added two more types: LayoutConfig and PageNjkAssets to avoid any
Fix some nits/bugs? along the way:
in Site:
await this.layoutManager.updateLayouts(filePath)
=>
await this.layoutManager.updateLayouts(filePathArray)

and in Layout.ts

generateDependencies(pageSources.getDynamicIncludeSrc(),
this.includedFiles)
=>
await this.config.externalManager.generateDependencies(
pageSources.getDynamicIncludeSrc(),
this.includedFiles,
this.layoutUserScriptsAndStyles);
}

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Migrate layout to Typescript

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@codecov

codecov Bot commented Mar 10, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 57.14286% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 48.93%. Comparing base (37d2c18) to head (010d9e1).

Files Patch % Lines
packages/core/src/Layout/Layout.ts 46.15% 7 Missing ⚠️
packages/core/src/Layout/LayoutManager.ts 50.00% 7 Missing ⚠️
packages/core/src/Page/index.ts 25.00% 3 Missing ⚠️
packages/core/src/Site/index.ts 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
+ Coverage   48.90%   48.93%   +0.02%     
==========================================
  Files         124      124              
  Lines        5247     5250       +3     
  Branches     1112     1112              
==========================================
+ Hits         2566     2569       +3     
- Misses       2373     2376       +3     
+ Partials      308      305       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yucheng11122017
yucheng11122017 reviewed Mar 11, 2024
View reviewed changes

@yucheng11122017 yucheng11122017 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yiwen101 ! Thank you very much. This looks like excellent work.

One vvv minor nit and I think we should be good to go.

Comment thread packages/core/src/Site/index.ts Outdated
itsyme
itsyme reviewed Mar 11, 2024
View reviewed changes
Comment thread packages/core/src/Layout/Layout.ts
@yiwen101 yiwen101 force-pushed the layout_ts branch 2 times, most recently from 4ac13fa to ae35f85 Compare March 13, 2024 09:52
@yiwen101

yiwen101 commented Mar 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@kaixin-hc @itsyme @EltonGohJH @yucheng11122017
Hi, there is a strange behaviours. There should not be any outdated changes to master from branch, but it says "This branch is out-of-date with the base branch"

This is what I did:
// my master branch is up to date, currently at the layout_ts branch

git merge master
git rebase -i HEAD~2
pick ......
squash ....
squash ...
...
git push -f

Update: issue resolved by simply pressing update branch with rebase from github page (@kaixin-hc thank you so much for telling me this trick in previous PR!!!)

But still curious which of my steps before is wrong.

Thanks.

@kaixin-hc

kaixin-hc commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

Hi Yiwen! Instead of git merge master followed by git merge rebase - you should be able to git rebase master, which would properly rebase it off of master.

What you did was rebase the last two commits on the same branch - I'm not totally sure what that does TBH, its not a common use and perhaps not an intended one

kaixin-hc
kaixin-hc approved these changes Mar 14, 2024
View reviewed changes

@kaixin-hc kaixin-hc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yiwen101

yiwen101 commented Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

Hi Yiwen! Instead of git merge master followed by git merge rebase - you should be able to git rebase master, which would properly rebase it off of master.

What you did was rebase the last two commits on the same branch - I'm not totally sure what that does TBH, its not a common use and perhaps not an intended one

Noted, thank you for the guidance.

@itsyme

itsyme commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

Hi Yiwen! Instead of git merge master followed by git merge rebase - you should be able to git rebase master, which would properly rebase it off of master.

What you did was rebase the last two commits on the same branch - I'm not totally sure what that does TBH, its not a common use and perhaps not an intended one

Just to add on I think what the interactive rebase did was just to squash the previous 2 commits into the current commit @yiwen101

itsyme
itsyme approved these changes Mar 14, 2024
View reviewed changes

@itsyme itsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the effort @yiwen101!

@yiwen101

yiwen101 commented Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

Hi Yiwen! Instead of git merge master followed by git merge rebase - you should be able to git rebase master, which would properly rebase it off of master.
What you did was rebase the last two commits on the same branch - I'm not totally sure what that does TBH, its not a common use and perhaps not an intended one

Just to add on I think what the interactive rebase did was just to squash the previous 2 commits into the current commit @yiwen101

Noted, thank you for the insight!

@kaixin-hc kaixin-hc merged commit f0aca6f into MarkBind:master Mar 14, 2024
kaixin-hc added a commit that referenced this pull request Mar 14, 2024
@kaixin-hc
Revert "Migrate layout to Typescript (#2452)"
c5f4b2a 
This reverts commit f0aca6f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EltonGohJH EltonGohJH EltonGohJH left review comments
@kaixin-hc kaixin-hc kaixin-hc approved these changes
@itsyme itsyme itsyme approved these changes
@yucheng11122017 yucheng11122017 Awaiting requested review from yucheng11122017

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yiwen101 @kaixin-hc @itsyme @EltonGohJH @yucheng11122017